Skip to content

Run Ask in the lane D agent container - #54

Open
mchwang wants to merge 31 commits into
docs/reconcile-plan-with-codefrom
ask-in-container
Open

mchwang wants to merge 31 commits into
docs/reconcile-plan-with-codefrom
ask-in-container

Conversation

@mchwang

@mchwang mchwang commented Sep 26, 2026 •

Copy link
Copy Markdown
Contributor

Stacked on #52 (base docs/reconcile-plan-with-code). When #52 merges, GitHub retargets this PR to main.

Summary

Ask no longer runs the claude or codex CLI on the host. It runs in lane D's container, in the read-only questions phase. This closes the interim R1 exception that #52 recorded.

Before After
Where the agent runs Host process, temporary folder, your environment Lane D container: read-only root, no capabilities, bounded storage
Code it can see Only the context in the prompt That context, plus a read-only clone of the reviewed snapshot head at /work
Commands Turned off by CLI flags questions phase: read, list and search only, enforced by lane D's policy
Network Host network Vendor-only proxy
Sign-in Host CLI login or keychain CLAUDE_CODE_OAUTH_TOKEN (from claude setup-token) or Codex auth.json, read from the environment and never stored

How it works

  • runner/question-container.ts builds the image once per worker, clones the head, allocates storage, captures the invocation (no approved argv), and starts the adapter. Storage is removed only after settled, as the D5 handoff requires.
  • runner/question-worker.ts: every lane D entry point is synchronous (Docker and Git calls), so a worker thread owns them and the review server stays responsive. Lane D's trust registries live in that one worker.
  • runner/question-agent.ts (QuestionWorker): a question settles only when the worker reports that the container and storage are gone, so a cancelled or timed-out container can't outlive the state that started it.
  • There is no host fallback. A missing token, a missing auth.json or no Docker fails with a message that names what's missing. Provider failures now include the vendor's short reason, for example 401 Invalid bearer token.

Behavior changes for users

  • Ask, including npm run demo, needs Docker plus a token or auth.json. The first question builds the agent image. If that takes longer than the two-minute question deadline, the question fails; Docker keeps the finished layers, and a retry continues from them.
  • A repository larger than the 512 MiB tmpfs allocation fails closed.

Tests

  • test/question-agent.test.ts, orchestration with injected lane D dependencies:
    • the invocation uses the questions phase with no argv and the reviewed head;
    • the token goes only to the adapter;
    • the input folder is read-only and contains only schema.json;
    • storage is released after settlement, never before;
    • each cancel reason maps to the right stop reason;
    • missing credentials fail before any Docker or Git work;
    • the image is built once;
    • storage is released when setup fails after allocation.
  • The worker bridge runs on a real Worker with a stub: answer passthrough, cancellation that waits for the worker's reply, and crash recovery.
  • test/questions.test.ts: the agent receives the configured repository and snapshot head. The timeout-then-retry regression required by AGENTS.md is already covered by "keeps cancelled invocations tracked until they settle".
  • test/agent-question.test.ts (real Docker; added to the Agent isolation workflow and excluded from main CI like the other Docker suites):
    • with a fake token, the real worker gets through image build, clone, storage, network and container start, and receives Anthropic's 401. Only a request that left the container through the vendor proxy can get that response;
    • the live case (CODEBOOST_RUN_AUTH_PROBES=1) asks for a random word that exists only in a file in the repository, so a correct answer shows the agent read /work.

Validation

Evidence on the merged head (after merging the updated #52 branch, which includes main at 5e97aa3):

  • npm run typecheck: passed.
  • Unit tests with CI's excludes: 464/464 passed.
  • npm run test:browser: 58/58 passed.
  • test/agent-question.test.ts on local Docker (earlier head f5b2f09): passed, with the live case skipped. CI's real-docker job also passed.
  • Self-review of the full diff under AGENTS.md. The live Claude case has not run; it needs a real CLAUDE_CODE_OAUTH_TOKEN.

Ownership note

The plan assigns this move to lane F. F1 (#49, merged) was docs only. F1c (#57) also edits runner/questions.ts, so whichever of #54 and #57 lands second must be rebased, fitting Ask's worker shutdown (Questions.close → QuestionWorker.close) into F1c's shutdown order. This PR edits shared files owned by the integration owner (runner/questions.ts, web/public/app.js, .github/workflows/ci.yml, .github/workflows/agent-isolation.yml), so it needs that owner's review.

Review-lesson audit

No review findings yet.

🤖 Generated with Claude Code

Ask used to run the claude/codex CLI on the host with each CLI's own
restrictions, an interim exception to R1. It now uses lane D's invocation
boundary in the read-only "questions" phase: a clone of the reviewed
snapshot head at /work, no commands, vendor-only network, and no other
host files. There is no host fallback.

- runner/question-container.ts: build image, clone, allocate bounded
  storage, capture, start the Claude/Codex adapter; release storage only
  after the invocation settles. Deps are injectable for unit tests.
- runner/question-worker.ts: lane D setup is synchronous, so a worker
  thread owns it and the review server stays responsive.
- runner/question-agent.ts: QuestionWorker bridge; a question settles only
  when the worker reports the container and storage are gone.
- Credentials come from the environment only: CLAUDE_CODE_OAUTH_TOKEN for
  Claude, CODEBOOST_CODEX_AUTH_FILE or CODEX_HOME/auth.json for Codex.
- Provider failures include the vendor's short message (e.g. a 401).
- test/agent-question.test.ts runs the path on real Docker (Agent
  isolation workflow); its live case needs the auth-probe credentials.
- Plan, README, Settings copy and implementation docs updated; the R1
  exception is closed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings September 26, 2026 08:20

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Critical and moderate findings remain around invocation integrity, cleanup recovery, worker crashes, and CI coverage.

Review effort: Lite
Findings: 1 High severity · 2 Medium severity

Open (3)
What changed in this PR

Moves Ask from host CLI execution into lane D’s isolated Docker container workflow.

Changes:

  • Adds worker-thread orchestration and container-backed Claude/Codex execution.
  • Adds credential, storage, cancellation, cleanup, and Docker integration tests.
  • Updates UI, CLI guidance, documentation, and CI workflows.

Unresolved findings concern invocation identity/context, resource cleanup and crash recovery, and workflow path coverage.

File Summary
web/​public/​app.js Updated settings guidance
web/​cli.ts Updated CLI guidance
test/​questions.test.ts Question lifecycle coverage
test/​question-agent.test.ts Orchestration and worker tests
test/​fixtures/​question-worker-stub.ts Worker test fixture
test/​agent-question.test.ts Real Docker integration tests
runner/​questions.ts Ask orchestration and scope
runner/​question-worker.ts Lane D worker bridge
runner/​question-container.ts Container lifecycle and cleanup
runner/​question-agent.ts Worker lifecycle and cancellation
README.md Containerized Ask documentation
docs/​implementation/​read-only-review.md Superseded host-adapter documentation
docs/​implementation/​agent-isolation.md Ask isolation documentation
docs/​designs/​codeboost-plan-indexed-review.md Updated roadmap and safety status
.github/​workflows/​ci.yml Excludes Docker tests from main CI
.github/​workflows/​agent-isolation.yml Runs Ask isolation tests

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread runner/question-container.ts Outdated
Comment thread runner/question-agent.ts Outdated
Comment thread runner/question-container.ts
…nto ask-in-container

# Conflicts:
#	docs/designs/codeboost-plan-indexed-review.md
Copilot AI review requested due to automatic review settings September 26, 2026 09:44

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Unresolved lifecycle, cleanup, cancellation, identity, permissions, result-validation, and CI coverage findings remain.

Review effort: Lite
Findings: 2 High severity · 3 Medium severity

Open (5)

Comment thread runner/question-container.ts
Comment thread runner/question-container.ts Outdated
…nto HEAD

# Conflicts:
#	docs/designs/codeboost-plan-indexed-review.md
Copilot AI review requested due to automatic review settings September 26, 2026 17:16

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Four moderate findings remain around cancellation, crash recovery, cleanup recovery, and stderr diagnostics.

Review effort: Lite
Findings: 2 High severity · 3 Medium severity

Open (5)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings September 26, 2026 17:31

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Moderate resource-lifecycle, worker, container, and workflow coverage issues remain unresolved.

Review effort: Lite
Findings: 2 High severity · 4 Medium severity

Open (6)

Comment thread .github/workflows/agent-isolation.yml
- Reuse the persisted answer attempt as the invocation attempt, and the
  note's contextId as referencedCodeHash. Accept a result only when its
  attempt and context match the captured invocation and the worker reply
  carries the same attempt.
- Treat a missing exit code or any signal as a failure, not an answer.
- Keep task storage whose removal Docker did not confirm, retry removal
  before the next question, and refuse Ask while any remains.
- After a worker crash, fail closed instead of starting a replacement:
  its containers and storage may still exist, and reclaiming them needs
  lane D's scoped recovery (#51 item 4).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings September 26, 2026 17:35
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Six moderate findings remain around cleanup recovery, admission coordination, settlement, and CI path coverage.

Review effort: Lite
Findings: 2 Medium severity

Open (2)
Resolved since last review (6)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Setup failures lose filesystem allocation ownership

runner/​question-container.ts:120

If prepareFilesystems creates volumes and then fails, filesystems is never assigned because the call has not returned. The allocator's cleanup can itself fail, but this caller has no allocation handle to put in RetainedStorage, so the labeled volumes/keeper can be abandoned and the next question cannot retry them. The allocation API needs to return/retain ownership for setup failures, not only for failures after line 120.

Comment thread runner/question-agent.ts Outdated
Comment thread runner/questions.ts Outdated
Copilot AI review requested due to automatic review settings September 26, 2026 17:39

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Unresolved moderate issues affect provider diagnostics, bounded storage, cleanup recovery, error preservation, and cancellation/shutdown behavior.

Review effort: Lite
Findings: 2 Medium severity

Open (2)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Failed filesystem setup leaks untracked resources

runner/​question-container.ts:120

prepareTaskFilesystems can create volumes and the keeper before a later allocation step fails; if its internal cleanup cannot confirm removal, it throws without returning a TaskFilesystems. Because filesystems is assigned only after this call returns, this path cannot add the allocation to RetainedStorage, so the remaining labeled volumes/keeper are forgotten and a later question can start. The allocator needs to return/retain an authenticated handle on setup failure or provide scoped recovery before this path is used.

Terminating the question worker dropped its only handles to storage
that Docker had not removed. Shutdown now asks the worker for one last
bounded removal, records anything still unremoved beside the review
database, and the next session refuses Ask, with the removal commands,
while any recorded container or volume still exists. The record clears
itself once they are gone; an unreadable record or unreachable daemon
keeps Ask off. Removal through D waits for its recovery handles (#51).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings September 26, 2026 17:44
When task storage setup fails and lane D cannot confirm its own cleanup,
D returns no handle, so Ask cannot name the leftovers. Ask now counts
the failure, stays off for the session, records it at shutdown, and
after a restart stays off while any io.codeboost.task-storage container
or volume exists. Caller-provided allocation IDs (#51 item 3) would let
Ask name these resources instead.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment thread runner/question-agent.ts Outdated
Comment thread runner/question-agent.ts Outdated
Comment thread runner/question-container.ts Outdated
Comment thread runner/question-leftovers.ts Outdated
Comment thread runner/question-leftovers.ts Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Unresolved moderate findings and a documentation inconsistency remain.

Review effort: Lite
Findings: 1 Medium severity

Open (1)
Resolved since last review (2)

Comment thread runner/question-leftovers.ts
The durable record sits beside the database path, so after a rename a
new process would not see roots recorded under the old name. Each Ask
root now carries an .owner stamp naming its lock, written under a
preparation name before the folder is renamed into place. The first
check of a process deletes unrecorded codeboost-ask-* folders whose
owner lock is free and leaves those whose owner is still running.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings September 26, 2026 19:40

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Unresolved critical and moderate findings remain in container preflight, cleanup, worker deadlines, and CI coverage.

Review effort: Lite
Findings: 3 High severity

Open (3)
Resolved since last review (1)

Comment thread runner/question-container.ts Outdated
Comment thread runner/question-leftovers.ts
Comment thread runner/question-leftovers.ts
- The pre-clone Git measurement now uses the same hardening as lane D's
  clone: GIT_NO_LAZY_FETCH, protocol.allow=never, no replace objects,
  no hooks, no graft file, no submodule recursion.
- An .owner stamp is probed only when it names a codeboost lock file in
  the temp directory; anything else counts as no owner, so a lookalike
  root cannot make startup open or create a file elsewhere. Every
  ledger's lock now lives there under that name.
- Regression test: startup still scans Docker after deleting a recorded
  root (the reported bypass does not reproduce).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings September 26, 2026 19:53

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The cancellation race is unresolved, and recovery-label documentation needs correction.

Review effort: Lite
Findings: None

Resolved since last review (3)

The docs and one Ask error message still said io.codeboost.task-storage,
but the scan checks containers, volumes and networks labelled
io.codeboost.allocation, io.codeboost.invocation or io.codeboost.egress.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings September 26, 2026 20:01

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Five unresolved findings remain, including one critical temporary-file handling issue.

Review effort: Lite
Findings: 1 High severity

Open (1)

Comment thread runner/question-leftovers.ts Outdated
The record was written via a predictable <record>.<pid>.tmp name with
the default "w" flag, so a planted link at that name would be followed
and its target overwritten. Use a random name opened with "wx" and
delete it if the write or rename fails.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings September 26, 2026 20:12

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Unresolved critical and moderate findings remain in cancellation, cleanup durability, storage bounds, startup cleanup, and shutdown admission.

Review effort: Lite
Findings: 2 High severity

Open (2)
Resolved since last review (1)

Comment thread runner/question-agent.ts Outdated
Comment thread runner/questions.ts
…ails

- The server stops question admission in the same turn it starts
  shutting down, so a request still arriving cannot start an agent or a
  container worker during the drain. A question it saved gets a
  retryable "Server stopped" answer without any agent starting; the
  existing drain test now expects zero agent calls instead of one
  started-then-cancelled call.
- If the final release report cannot be saved, the worker's root is no
  longer deleted: it stays on disk and in the record for the next
  session, and Docker leftovers remain covered by the startup scan.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings September 26, 2026 20:34

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Unresolved critical cleanup/lock failure and moderate provider-diagnostic gap require changes.

Review effort: Lite
Findings: 1 High severity

Open (1)
Resolved since last review (2)

Comment thread runner/question-agent.ts
After a failed release-report write the worker thread has stopped and
the root is already recorded, so let go of the root in this process:
it stays on disk and in the record for the next check, and close() can
release the per-review lock instead of holding it for the process.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings September 26, 2026 20:42

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Unresolved lifecycle, cancellation, resource-ownership, and shutdown error-handling issues remain.

Review effort: Lite
Findings: 1 High severity

Open (1)
Resolved since last review (1)

Comment thread runner/question-agent.ts Outdated
A rejected terminate() no longer counts as a stopped thread: the Ask
root stays on disk and in the record, and the lock stays held, instead
of being removed while the worker may still be alive. Later cleanup runs
only after a termination that actually settled.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings September 26, 2026 20:52

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Two unresolved moderate issues remain in cancellation handling and shared Docker-resource isolation.

Review effort: Lite
Findings: None

Resolved since last review (1)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants